feat(dynamic-sampling): make docs simpler and easier to understand#17748
feat(dynamic-sampling): make docs simpler and easier to understand#17748shellmayr wants to merge 16 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| --- | ||
| title: Fidelity and Biases | ||
| title: Biases | ||
| sidebar_order: 2 | ||
| og_image: /og-images/application-architecture-dynamic-sampling-fidelity-and-biases.png | ||
| og_image: /og-images/application-architecture-dynamic-sampling-biases.png | ||
| --- |
There was a problem hiding this comment.
Bug: Renaming fidelity-and-biases.mdx to biases.mdx broke existing redirect chains and internal links because the necessary redirects were not added to middleware.ts.
Severity: MEDIUM
Suggested Fix
In middleware.ts, add two redirects. First, a redirect from the old final path /application-architecture/dynamic-sampling/fidelity-and-biases/ to the new path /application-architecture/dynamic-sampling/biases/. Second, a new redirect from the short path /dynamic-sampling/biases/ to the full path /application-architecture/dynamic-sampling/biases/ to fix the internal link.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: develop-docs/application-architecture/dynamic-sampling/biases.mdx#L1-L5
Potential issue: Renaming the file `fidelity-and-biases.mdx` to `biases.mdx` has broken
several navigation paths. First, the existing redirect chain for the old URL
(`/dynamic-sampling/fidelity-and-biases/` ->
`/application/dynamic-sampling/fidelity-and-biases/` ->
`/application-architecture/dynamic-sampling/fidelity-and-biases/`) now leads to a 404
page because the final destination was moved without an additional redirect. Second, the
'next page' link in `the-big-picture.mdx` points to the new short path
`/dynamic-sampling/biases/`, but no corresponding redirect was added in `middleware.ts`
to map it to the full path. Both external links and internal navigation will be broken.
Also affects:
develop-docs/application-architecture/dynamic-sampling/the-big-picture.mdx:99
Did we get this right? 👍 / 👎 to inform future reviews.
Dav1dde
left a comment
Dav1dde
left a comment
There was a problem hiding this comment.
This seems a lot easier to understand for people who haven't fully embraced DS (yet), it does skip over some details which before were mentioned (e.g. how Relay behaves under certain circumstances/edge cases). Likely, this is a good thing, as it may be better to actually confirm edge cases with the code instead.
Curious what others think!
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 29ba485. Configure here.
| 1. Rule `1` is matched against the DSC, since it is of type `trace`. The `samplingValue` is a `factor` with value `2.0`. | ||
| 2. Because rule `1` was a factor rule, the matching continues and rule `2` will again be matched against the DSC, since it is of type `trace`. The `samplingValue` is a `sampleRate`, thus the matching will stop and the sample rate will be computed as `2.0 * 0.5 = 1.0`, where `2.0` is the factor accumulated from the previous rule and `0.5` is the sample rate of the current rule. |
There was a problem hiding this comment.
Bug: The "Sampling Decision" example incorrectly states Rule 1 matches the DSC, as its trace.transaction condition doesn't align with the provided DSC transaction value.
Severity: LOW
Suggested Fix
Update the "Sampling Decision" example to be logically consistent. Either change the DSC's transaction value to "/world" to make Rule 1 match, or adjust the explanation to correctly state that Rule 1 does not match and the final sample rate is 0.5 based on Rule 2 alone.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
develop-docs/application-architecture/dynamic-sampling/architecture.mdx#L127-L128
Potential issue: The documentation for Dynamic Sampling contains a logically
inconsistent example under "Sampling Decision". Rule 1, a `trace` rule, has a condition
`trace.transaction == "/world"`. However, the Dynamic Sampling Context (DSC) it is
evaluated against has `transaction: "/hello"`. The documentation text at lines 127-128
incorrectly claims that Rule 1 matches, leading to a final sample rate calculation of
1.0. The correct rate, based only on the matching Rule 2, should be 0.5. This error
makes the example misleading and factually incorrect.
3 participants
Closes TET-2348